Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(collector-webhook): ensure stabilizationWindowSeconds validation matches k8s.io/api/autoscaling/v2 requirements #3346

Merged
merged 3 commits into from
Oct 11, 2024
Merged

fix(collector-webhook): ensure stabilizationWindowSeconds validation matches k8s.io/api/autoscaling/v2 requirements #3346

merged 3 commits into from
Oct 11, 2024

Conversation

onematchfox
Copy link
Contributor

Description:

This PR:

  • ensures that the validation of autoscaler.behaviour.scale[Down|Up].stabilizationWindowSeconds matches the requirements of the upstream module k8s.io/api/autoscaling/v2/types.go
  • adjusts the error message to report that scale[Down|Up].stabilizationWindowSeconds in specific is invalid rather than referring to the entire object.

Link to tracking Issue(s):

Testing:

Existing tests have been updated to match the new requirements.

@onematchfox
fix(collector-webhook): ensure stabilizationWindowSeconds validatio…
d43d0a1 
…n matches `k8s.io/api/autoscaling/v2` requirements
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Oct 11, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@onematchfox onematchfox marked this pull request as ready for review October 11, 2024 08:56
@onematchfox onematchfox requested a review from a team as a code owner October 11, 2024 08:56
@swiatekm
Copy link
Contributor

Thank you for the contribution! Could you add a changelog entry for this fix? make chlog-new will generate a template you can fill out.

The change itself looks good to me. One thing I don't really understand is why the struct we're importing from k8s.io/api/autoscaling doesn't come with the right validations in the first place. If we declared it on our own, we could easily validate it via OpenAPI in the CRD, not sure why Kubernetes itself can't do that.

@onematchfox
Copy link
Contributor Author

Could you add a changelog entry for this fix? make chlog-new will generate a template you can fill out.

Done. Sorry I missed that.

One thing I don't really understand is why the struct we're importing from k8s.io/api/autoscaling doesn't come with the right validations in the first place. If we declared it on our own, we could easily validate it via OpenAPI in the CRD, not sure why Kubernetes itself can't do that.

Yeah, I'm not sure about that either. All I can add is that in trying to understand why this code was there I tracked it back to #1077.

@swiatekm swiatekm merged commit 7a79233 into open-telemetry:main Oct 11, 2024
35 checks passed
@alexkruc alexkruc mentioned this pull request Dec 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swiatekm swiatekm swiatekm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect validation and error message of stabilizationWindowSeconds in autoscaler behaviour
2 participants
@onematchfox @swiatekm